feat(plugin config): add dependency for delete plugin config #6092

zaunist · 2022-01-13T14:33:54Z

What this PR does / why we need it:

When a plugin config is referencing by any route, it should not be deleted.

example:

create plugin config:

curl http://127.0.0.1:9180/apisix/admin/plugin_configs/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -i -d '
{                                                                                                                                                                                                         
    "desc": "吾乃插件配置1",                                                                                                                                                                              
    "plugins": {                                                                                                                                                                                          
        "limit-count": {                                                                                                                                                                                  
            "count": 2,                                                                                                                                                                                   
            "time_window": 60,                                                                                                                                                                            
            "rejected_code": 503                                                                                                                                                                          
        }                                                                                            
    }                                                                                                                                                                                                     
}'

bounding route to plugin config:

curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -i -d '
{
    "uris": ["/index.html"],
    "plugin_config_id": 1,
    "upstream": {
        "type": "roundrobin",
        "nodes": {
            "39.97.63.215:80": 1
        }
    }
}'

Then, delete plugin config, apisix will return 400 error.

▶ curl -X DELETE http://127.0.0.1:9080/apisix/admin/plugin_configs/1  -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1'
{"error_msg":"can not delete this plugin config, route [1] is still using it now"}

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

zaunist · 2022-01-16T10:50:13Z

cc @tzssangglass

spacewander · 2022-01-16T12:25:41Z

t/admin/delete-plugin-configs.t

@@ -0,0 +1,174 @@
+#


Could we add it to t/admin/plugin-configs.t?

We should not add more new delete-* files. For example, the new case in #4575 is added in proto3.t.
I also submit a PR to remove the stale cases: #6121

spacewander · 2022-01-16T12:26:48Z

t/admin/delete-plugin-configs.t

+                        }
+                    }
+                }]],
+                [[{


We don't need to check the response in every test.

spacewander · 2022-01-16T12:28:42Z

apisix/admin/plugin_config.lua

+    end
+
+    local routes, routes_ver = get_routes()
+    core.log.info("routes: ", core.json.delay_encode(routes, true))


Is it necessary to dump all the routes?

I did it with reference to the processing of upstream. At the same time, I also considered whether to logging all route information. If it is not log here, does it also need to be modified in upstream?

apisix/apisix/admin/upstreams.lua

Line 130 in c38e94f

core.log.info("routes: ", core.json.delay_encode(routes, true))

As the size of Nginx log is limited, we can't log all the routes. Let's remove it.

spacewander · 2022-01-17T02:59:51Z

apisix/admin/plugin_config.lua

+    end
+
+    local routes, routes_ver = get_routes()
+    core.log.info("routes: ", core.json.delay_encode(routes, true))


As the size of Nginx log is limited, we can't log all the routes. Let's remove it.

spacewander · 2022-01-17T03:00:37Z

t/admin/plugin-configs.t

+            ngx.say(body)
+        }
+    }
+--- request


We can remove the duplicate sections which are set in the beginning.

spacewander · 2022-01-17T03:01:16Z

t/admin/plugin-configs.t

+GET /t
+--- response_body
+passed
+--- no_error_log


bisakhmondal

lgtm%nits

bisakhmondal · 2022-01-17T04:54:02Z

t/admin/plugin-configs.t

+--- config
+    location /t {
+        content_by_lua_block {
+            ngx.sleep(0.3)


Hi, is there any reason for which we need to add this 0.3-sec sleep?
Thanks.

The current step depends on the previous steps, so set a short time for etcd to sync data.

bisakhmondal · 2022-01-17T04:54:08Z

t/admin/plugin-configs.t

+--- config
+    location /t {
+        content_by_lua_block {
+            ngx.sleep(0.3)


bisakhmondal · 2022-01-17T04:54:14Z

t/admin/plugin-configs.t

+--- config
+    location /t {
+        content_by_lua_block {
+            ngx.sleep(0.3)


spacewander · 2022-01-17T07:24:35Z

I found the CI is broken with msg:

nginx: [emerg] "listen" directive is not allowed here in /home/runner/work/apisix/apisix/t/servroot/conf/nginx.conf:202

https://github.com/apache/apisix/runs/4836910403?check_suite_focus=true

See apache#6092 (comment)

zaunist · 2022-01-17T08:17:55Z

I found the CI is broken with msg:
nginx: [emerg] "listen" directive is not allowed here in /home/runner/work/apisix/apisix/t/servroot/conf/nginx.conf:202
https://github.com/apache/apisix/runs/4836910403?check_suite_focus=true

Thanks for your careful, but I don't know how to slove this problem. Any one can help me to do this ?

spacewander · 2022-01-17T09:38:49Z

Usually, it is because of a malformed config section. Could you check the t/servroot/conf/nginx.conf?

…te_plugin_dependency

zaunist · 2022-01-18T05:47:55Z

Usually, it is because of a malformed config section. Could you check the t/servroot/conf/nginx.conf?

Got it, and CI passed now.

zaunist added 2 commits January 13, 2022 22:26

feat(plugin config): add dependency for delete plugin config

65b7d29

test: add test case and fix ci failed

0b2a23e

zaunist marked this pull request as ready for review January 16, 2022 10:16

juzhiyuan requested a review from bisakhmondal January 16, 2022 10:32

tokers approved these changes Jan 16, 2022

View reviewed changes

spacewander reviewed Jan 16, 2022

View reviewed changes

tzssangglass approved these changes Jan 17, 2022

View reviewed changes

fix: move delete_*.t to plugin_configs.t

311efe2

zaunist requested a review from spacewander January 17, 2022 03:00

spacewander reviewed Jan 17, 2022

View reviewed changes

fix: remove core.log.info

781080c

bisakhmondal approved these changes Jan 17, 2022

View reviewed changes

fix: ci failed

6eada5d

zaunist requested a review from spacewander January 17, 2022 06:40

spacewander added a commit to spacewander/incubator-apisix that referenced this pull request Jan 17, 2022

test: fix the CI failure detection when rerunning the tests

30f9d4f

See apache#6092 (comment)

spacewander mentioned this pull request Jan 17, 2022

test: fix the CI failure detection when rerunning the tests #6129

Merged

4 tasks

zaunist added 3 commits January 18, 2022 09:34

Merge remote-tracking branch 'remotes/upstream/master' into feat/dele…

3ef015e

…te_plugin_dependency

fix: ci failed

f333b2e

chore: code style

2dc9ba6

spacewander approved these changes Jan 18, 2022

View reviewed changes

spacewander merged commit 3773f26 into apache:master Jan 18, 2022

zaunist deleted the feat/delete_plugin_dependency branch January 18, 2022 07:01

zaunist mentioned this pull request Jan 19, 2022

chore: remove log routes info #6152

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin config): add dependency for delete plugin config #6092

feat(plugin config): add dependency for delete plugin config #6092

zaunist commented Jan 13, 2022 •

edited

Loading

zaunist commented Jan 16, 2022

spacewander Jan 16, 2022

spacewander Jan 16, 2022

zaunist Jan 17, 2022

spacewander Jan 16, 2022

zaunist Jan 17, 2022

spacewander Jan 16, 2022

zaunist Jan 17, 2022

spacewander Jan 17, 2022

spacewander Jan 17, 2022

spacewander Jan 17, 2022

spacewander Jan 17, 2022

bisakhmondal left a comment •

edited

Loading

bisakhmondal Jan 17, 2022

zaunist Jan 17, 2022

bisakhmondal Jan 17, 2022

bisakhmondal Jan 17, 2022

spacewander commented Jan 17, 2022

zaunist commented Jan 17, 2022

spacewander commented Jan 17, 2022

zaunist commented Jan 18, 2022

feat(plugin config): add dependency for delete plugin config #6092

feat(plugin config): add dependency for delete plugin config #6092

Conversation

zaunist commented Jan 13, 2022 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

zaunist commented Jan 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisakhmondal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander commented Jan 17, 2022

zaunist commented Jan 17, 2022

spacewander commented Jan 17, 2022

zaunist commented Jan 18, 2022

zaunist commented Jan 13, 2022 •

edited

Loading

bisakhmondal left a comment •

edited

Loading